Skip to content

Conversation

@xDefcon
Copy link

@xDefcon xDefcon commented Oct 21, 2025

The problem is showcased here: #339

Problem

The parseHeight method in Buildings.java did not handle common OSM tagging mistakes where commas are used as decimal separators instead of periods (eg. height=89,10 instead of height=89.10).

This caused incorrect parsing:

  • "89,10" was parsed as 8910 meters instead of 89.10 meters
  • "89,1 m" was parsed as 891 meters instead of 89.1 meters

According to the OSM height documentation, commas are incorrect but appear frequently in real-world OSM data.

Problem example as in discussion 339:
Screenshot 2025-10-21 194416

Solution

Note: this is similar to the solution that JOSM used here: https://josm.openstreetmap.de/ticket/15719

Added a sanitizeHeightValue() method that:

  1. Detects commas followed by 1-2 digits (with optional unit suffix like m)
  2. Replaces the comma with a period to create valid decimal notation
  3. Preserves actual thousand separators (e.g., "1,234" remains unchanged)

Solution example:
Screenshot 2025-10-21 194300

Changes

Modified Files

  • src/main/java/com/protomaps/basemap/layers/Buildings.java
    • Added sanitizeHeightValue()
    • Updated parseHeight() to sanitize both osmHeight and osmMinHeight parameters

Test Coverage

  • src/test/java/com/protomaps/basemap/layers/BuildingsTest.java
    • Added sanitizeHeightValue() test with proper test cases
    • Added parseHeightWithCommaDecimalSeparator() integration test

Examples

Input Before After
"89,10" 8910.0 89.10
"89,1" 891.0 89.1
"89,1 m" 891.0 89.1
"89,1m" 891.0 89.1
"89.10" 89.10 89.10 (unchanged)
"1,234" 1234.0 1234.0 (unchanged)

@xDefcon
Copy link
Author

xDefcon commented Oct 29, 2025

@bdon Hello, do you have any time to review this change? Thank you 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant